Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngModel, input): improve handling of built-in named parsers #16347

Merged
merged 1 commit into from
Dec 4, 2017

Conversation

Narretz
Copy link
Contributor

@Narretz Narretz commented Nov 27, 2017

This commit changes how input elements use the private $$parserName
property on the ngModelController to name parse errors. Until now,
the input types (number, date etc.) would set $$parserName when
the inputs were initialized, which meant that any other parsers on
the ngModelController would also be named after that type. The
effect of that was that the $error property and the ng-invalid-...
class would always be that of the built-in parser, even if the custom
parser had nothing to do with it.

The new behavior is that the $$parserName is only set if the actual
parser is invalid, i.e. returns undefined.

Also, $$parserName has been removed from input[email] and input[url],
as these types do not have a built-in parser anymore.

BREAKING CHANGE:

Custom parsers that fail to parse on input types email, url, date, month, time,
datetime-local, week, do no longer set $error[inputType], and no longer set the class
ng-invalid-[inputType]. Instead, they set $error.parse and ng-invalid-parse.

Closes #14292
Closes #10076

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
bugfix

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change?
Yes

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Fix/Feature: Docs have been added/updated
  • Fix/Feature: Tests have been added; existing tests pass

Other information:
This is a general fix to #10076 and also incorporates the changes in #14292.
While it's not a very common problem, I always found this section of the API very strange, especially since it uses an undocumented API (so if you stumble on it, it's even more difficult to find out what causes it).
Since #14292 is already a very small BC I thought a general solution to this issue would be welcome.
Note that I will include tests for the other input types if this implementation is approved.

Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM, except:

  1. AFAICT, the breaking change can potentialy affect all input types, not just email, url, date/month/time/datetime-local/week, as mentioned in the commit message.

  2. I don't think it properly addresses the problem of synonymous parsers and validators (see refactor(input): remove $$parserName from email, url #14292 (comment) for more details).

But overall, I think this is a nice change 💯

@Narretz
Copy link
Contributor Author

Narretz commented Dec 1, 2017

AFAICT, the breaking change can potentialy affect all input types, not just email, url, date/month/time/datetime-local/week, as mentioned in the commit message.

Hm, I don't see how. Only these input types have parsers that rely on the (private) $$parserName property. Sure, somebody could have ued it for their own parser, but this was not documented or supported, and mentioning it in the BC notice might confuse people.

I don't think it properly addresses the problem of synonymous parsers and validators (see #14292 (comment) for more details).

The original original issue (#14249) was about duplicated class setting, that is right. However, this was fixed in ngAnimate. Now we could optimize this in ngModel before the classes are set, but I don't think this is really related to this issue .. or is it?? 😱

@gkalpak
Copy link
Member

gkalpak commented Dec 1, 2017

AFAICT, the breaking change can potentialy affect all input types, not just email, url, date/month/time/datetime-local/week, as mentioned in the commit message.

Hm, I don't see how.

USers can add parsers to any input. Previously, a parse error on a number or range input, for example, would be reported as number or range (e.g. input.$error.number and ng-valid-number). Now, it is going to be parse, right? That can break styles and logic.

@gkalpak
Copy link
Member

gkalpak commented Dec 1, 2017

I don't think it properly addresses the problem of synonymous parsers and validators (see #14292 (comment) for more details).

The original original issue (#14249) was about duplicated class setting, that is right. However, this was fixed in ngAnimate.

It is still unexpected that we may add/remove classes twice. Might not cause an issue with ngAnimate, but is still something that we could avoid (e.g. as suggested in #14292 (comment)).

@Narretz
Copy link
Contributor Author

Narretz commented Dec 1, 2017

Users can add parsers to any input. Previously, a parse error on a number or range input, for example, would be reported as number or range (e.g. input.$error.number and ng-valid-number). Now, it is going to be parse, right? That can break styles and logic.

Ah, you are right. number and range also fall into this category.

It is still unexpected that we may add/remove classes twice. Might not cause an issue with ngAnimate, but is still something that we could avoid (e.g. as suggested in #14292 (comment)).

Is it unexpected if it doesn't have any effect apart from a small perf penalty? We can track this in a new issue.

This commit changes how input elements use the private $$parserName
property on the ngModelController to name parse errors. Until now,
the input types (number, date etc.) would set $$parserName when
the inputs were initialized, which meant that any other parsers on
the ngModelController would also be named after that type. The
effect of that was that the `$error` property and the `ng-invalid-...`
class would always be that of the built-in parser, even if the custom
parser had nothing to do with it.

The new behavior is that the $$parserName is only set if the built-in
parser is invalid i.e. returns `undefined`.

Also, $$parserName has been removed from input[email] and input[url],
as these types do not have a built-in parser anymore.

BREAKING CHANGE:

*Custom* parsers that fail to parse on input types "email", "url", "number", "date", "month",
"time", "datetime-local", "week", do no longer set `ngModelController.$error[inputType]`, and
the `ng-invalid-[inputType]` class. Also, custom parsers on input type "range" do no
longer set `ngModelController.$error.number` and the `ng-invalid-number` class.

Instead, any custom parsers on these inputs set `ngModelController.$error.parse` and
`ng-invalid-parse`.

Closes angular#14292
Closes angular#10076
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ngModel parse errors always use named parser
3 participants